Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pour_bottle?: Pour local bottles without sha256 #3105

Merged
merged 2 commits into from Aug 31, 2017

Conversation

sjackman
Copy link
Member

@sjackman sjackman commented Aug 29, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Pouring a local bottle for a formula without a bottle sha256
in the formula for that OS would unexpectedly install from
source instead.

@@ -85,7 +85,7 @@ def pour_bottle?(install_bottle_options = { warn: false })
return false if @pour_failed

bottle = formula.bottle
return false unless bottle
return false unless bottle || formula.local_bottle_path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return true if formula.local_bottle_path is on line 94 already so it feels weird to duplicate it here. Can this logic be moved around instead so it's only checked once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed return true if formula.local_bottle_path. Pouring a local bottle should still check that that bottle is compatible. --force-bottle may be used to pour a local bottle that is incompatible, as it is for remote bottles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, cool. Can you make this an if instead; unless with multiple conditions are hard to parse. After that should be 👍 to 🚢

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for the feedback, Mike.

Don't ignore f.pour_bottle? and compatible_cellar? when pouring
a local bottle. --force-bottle may be used to pour a local
bottle that is incompatible, as it is for remote bottles.
Pouring a local bottle for a formula without a bottle sha256
in the formula for that OS would unexpectedly install from
source instead.
@MikeMcQuaid MikeMcQuaid merged commit b2cd52d into Homebrew:master Aug 31, 2017
@MikeMcQuaid
Copy link
Member

Thanks again @sjackman!

@sjackman sjackman deleted the pour_local_bottle branch August 31, 2017 22:31
@sjackman
Copy link
Member Author

My pleasure. Thanks for the feedback, and thanks for merging, Mike.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants